Skip to content

fix: accumulate additive events in memory staging area#165

Open
yashhzd wants to merge 2 commits intomesa:mainfrom
yashhzd:fix/memory-staging-overwrite-137
Open

fix: accumulate additive events in memory staging area#165
yashhzd wants to merge 2 commits intomesa:mainfrom
yashhzd:fix/memory-staging-overwrite-137

Conversation

@yashhzd
Copy link
Contributor

@yashhzd yashhzd commented Mar 6, 2026

Summary

Fixes #137

The base Memory.add_to_memory() method uses a plain dict (step_content) where each event type maps to a single value. When multiple events of the same type occur within a single step (e.g., an agent receiving messages from several neighbors), subsequent calls silently overwrite previous entries, causing permanent data loss.

Changes

  • memory.py: Introduce ADDITIVE_EVENT_TYPES (frozenset({"message", "action"})) — events of these types are collected in a list rather than overwritten. Observations remain state-based (overwrite semantics preserved). Other types (plans, reasoning steps, etc.) also keep overwrite semantics.
  • memory.py (MemoryEntry.__str__): Updated to render list-valued content with numbered sub-entries for clean display.
  • st_memory.py / st_lt_memory.py: Updated get_communication_history() to iterate over list-valued message entries.
  • test_llm_agent.py: Updated test_apply_plan_adds_to_memory assertion to match the new list-based action storage.
  • New: tests/test_memory/test_memory_staging.py — 9 focused tests covering:
    • Single and multiple messages preserved in a list
    • Multiple actions preserved
    • Observations still overwrite (state-based)
    • Non-additive types still overwrite
    • Mixed types coexist correctly
    • MemoryEntry.__str__ formats lists properly
    • ShortTermMemory.get_communication_history handles list messages

Design Decision

Per the discussion in #76, communicative events (messages, actions) should be additive — an agent receiving five messages in one step should have all five recorded. Observations may remain state-based to prevent LLM context bloat.

The ADDITIVE_EVENT_TYPES set is a class attribute on Memory, making it easy for subclasses to extend with additional event types if needed.

Test Plan

  • All 271 existing tests pass
  • 9 new tests for staging area behavior
  • Pre-commit hooks pass (ruff check, ruff format, codespell)

Summary by CodeRabbit

Release Notes

  • New Features

    • Messages and actions within a step now accumulate as lists, preserving all events instead of overwriting.
    • Added support for rendering lists in memory content with indexed enumeration.
    • New async API for memory updates.
  • Bug Fixes

    • Improved communication history handling to properly display multiple messages per step.

The base Memory.add_to_memory() method used a plain dict for
step_content, so multiple events of the same type within a single
step overwrote each other silently.  An agent receiving three
messages in one step would only retain the last one.

Introduce ADDITIVE_EVENT_TYPES (message, action) whose entries are
collected in a list instead of being overwritten.  Observations
remain state-based (overwrite).

Update MemoryEntry.__str__, ShortTermMemory.get_communication_history,
and STLTMemory.get_communication_history to handle list-valued content.
@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.71%. Comparing base (4c0549e) to head (26e4c03).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
mesa_llm/memory/st_lt_memory.py 90.00% 1 Missing ⚠️
mesa_llm/memory/st_memory.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   90.08%   90.71%   +0.62%     
==========================================
  Files          19       19              
  Lines        1503     1572      +69     
==========================================
+ Hits         1354     1426      +72     
+ Misses        149      146       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add tests for:
- STLTMemory.get_communication_history with list and legacy messages
- ShortTermMemory.get_communication_history with legacy single-dict messages
- MemoryEntry.__str__ with list of non-dict items
- Legacy single-dict to list migration in add_to_memory
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 7, 2026

Walkthrough

The PR implements additive semantics for certain event types in the memory staging area to fix data loss when multiple events of the same type occur within a single simulation step. Messages and actions now accumulate in lists rather than overwriting each other. Supporting changes update memory rendering and communication history retrieval to handle list-valued content.

Changes

Cohort / File(s) Summary
Core Memory Logic
mesa_llm/memory/memory.py
Added ADDITIVE_EVENT_TYPES set containing "message" and "action" types. Modified add_to_memory to accumulate additive event types into lists across multiple calls within a step, while observation types retain overwrite semantics. Added aadd_to_memory async wrapper. Enhanced list rendering in memory content display.
Communication History Retrieval
mesa_llm/memory/st_memory.py, mesa_llm/memory/st_lt_memory.py
Refactored get_communication_history to handle messages stored as either single strings or lists, preserving multiple messages per step and ensuring robust formatting of both legacy and new list-based message storage.
Test Updates
tests/test_llm_agent.py
Updated test assertion to expect action events as lists accumulating multiple entries within a step, reflecting new additive semantics.
Comprehensive Test Coverage
tests/test_memory/test_memory_staging.py
Added extensive unit tests validating additive vs. overwrite semantics across event types, list rendering, communication history handling, legacy single-dict migration, and edge cases with ConcreteMemory test fixture.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PR #69: Also adds async wrapper aadd_to_memory to Memory class in mesa_llm/memory/memory.py with overlapping implementation details.

Suggested labels

bug

Suggested reviewers

  • sanika-n

Poem

🐰 A rabbit's ode to additive cheer,
No more lost messages, we hold them dear!
Lists grow where singles once did reign,
Each step's events preserved, none in vain. 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: introducing additive event semantics to prevent data loss in memory staging.
Description check ✅ Passed The description is comprehensive, covering changes, design decisions, and testing. It directly references the linked issue and includes a detailed test plan.
Linked Issues check ✅ Passed All requirements from issue #137 are met: additive event types (messages, actions) are collected in lists, observations retain overwrite semantics, and data loss is prevented.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing issue #137: memory staging improvements, helper method updates, test updates, and new comprehensive tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@colinfrisch
Copy link
Member

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
mesa_llm/memory/memory.py (2)

56-57: Minor formatting inconsistency for non-dict list items.

Line 57 appends a trailing colon after non-dict items ({item} :), but since these are values rather than key-value pairs, the colon appears unnecessary and inconsistent with how scalar values are displayed elsewhere (line 61 also has this pattern for non-list scalars).

💡 Suggested fix
                     else:
-                        lines.append(f"      [blue]└──[/blue] [cyan]{item} :[/cyan]")
+                        lines.append(f"      [blue]└──[/blue] [cyan]{item}[/cyan]")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mesa_llm/memory/memory.py` around lines 56 - 57, The list branch currently
appends non-dict items with a trailing colon using the lines.append call that
builds f"      [blue]└──[/blue] [cyan]{item} :[/cyan]"; remove the colon so
scalar list items render like other scalars (match the pattern used elsewhere
for non-list scalars, e.g., the similar append at the non-list scalar branch),
updating the string to omit " :" and ensure whitespace/formatting matches other
scalar lines.

143-146: Type annotation mismatch: frozenset assigned to set[str].

The annotation declares set[str] but the value is a frozenset. While this works at runtime, it's technically incorrect for static type checkers and could mislead subclasses about mutability.

💡 Suggested fix
-    ADDITIVE_EVENT_TYPES: set[str] = frozenset({"message", "action"})
+    ADDITIVE_EVENT_TYPES: frozenset[str] = frozenset({"message", "action"})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mesa_llm/memory/memory.py` around lines 143 - 146, The annotation for
ADDITIVE_EVENT_TYPES is incorrect: it declares set[str] but is assigned a
frozenset; update the type to match immutability by changing the annotation to
frozenset[str] (or typing.FrozenSet[str]) so ADDITIVE_EVENT_TYPES:
frozenset[str] = frozenset({"message", "action"}) and keep the existing value;
this ensures static type checkers and readers see the correct immutable type for
the constant.
mesa_llm/memory/st_memory.py (1)

97-111: LGTM with a consistency note.

The implementation correctly handles both list-valued and scalar message entries. However, EpisodicMemory.get_communication_history() (lines 150-160) directly accesses entry.content['message'] without checking if it's a list, unlike the updated approach here. If both classes are used with additive message storage, they should handle messages consistently to avoid formatting errors.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mesa_llm/memory/st_memory.py` around lines 97 - 111,
EpisodicMemory.get_communication_history currently assumes
entry.content['message'] is a scalar; change it to match the logic used in
get_communication_history of the short-term memory: first check that "message"
exists in entry.content, retrieve msgs = entry.content["message"], then if
isinstance(msgs, list) iterate and append each msg with the same formatting
(e.g., f"step {entry.step}: {msg}\n\n") else append the scalar message with the
same formatting; ensure you reference EpisodicMemory.get_communication_history
and use entry.step and entry.content consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@mesa_llm/memory/memory.py`:
- Around line 56-57: The list branch currently appends non-dict items with a
trailing colon using the lines.append call that builds f"      [blue]└──[/blue]
[cyan]{item} :[/cyan]"; remove the colon so scalar list items render like other
scalars (match the pattern used elsewhere for non-list scalars, e.g., the
similar append at the non-list scalar branch), updating the string to omit " :"
and ensure whitespace/formatting matches other scalar lines.
- Around line 143-146: The annotation for ADDITIVE_EVENT_TYPES is incorrect: it
declares set[str] but is assigned a frozenset; update the type to match
immutability by changing the annotation to frozenset[str] (or
typing.FrozenSet[str]) so ADDITIVE_EVENT_TYPES: frozenset[str] =
frozenset({"message", "action"}) and keep the existing value; this ensures
static type checkers and readers see the correct immutable type for the
constant.

In `@mesa_llm/memory/st_memory.py`:
- Around line 97-111: EpisodicMemory.get_communication_history currently assumes
entry.content['message'] is a scalar; change it to match the logic used in
get_communication_history of the short-term memory: first check that "message"
exists in entry.content, retrieve msgs = entry.content["message"], then if
isinstance(msgs, list) iterate and append each msg with the same formatting
(e.g., f"step {entry.step}: {msg}\n\n") else append the scalar message with the
same formatting; ensure you reference EpisodicMemory.get_communication_history
and use entry.step and entry.content consistently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3cee3f10-7388-4e91-98bc-3ecaacc8aac4

📥 Commits

Reviewing files that changed from the base of the PR and between 4c0549e and 26e4c03.

📒 Files selected for processing (5)
  • mesa_llm/memory/memory.py
  • mesa_llm/memory/st_lt_memory.py
  • mesa_llm/memory/st_memory.py
  • tests/test_llm_agent.py
  • tests/test_memory/test_memory_staging.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory staging area overwrites concurrent events of same type (leads to Data Loss)

3 participants